Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return task ARN from the handle command #34

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

rajadain
Copy link
Collaborator

@rajadain rajadain commented Dec 21, 2023

Overview

When running ecsmanage in a programmatic setting, it is useful to get the ARN of the task which it started, in addition to the URL being printed to the console. This doesn't break existing workflows, but adds the possibility of new ones.

Demo

image

Notes

This does add an addition line in the command output:

$ ./scripts/manage ecsmanage showmigrations

[+] Building 0.0s (0/0)                                                                                                                                                                                                  docker:desktop-linux
[+] Creating 1/0
 ✔ Container collab-for-children-database-1  Created                                                                                                                                                                                     0.0s
[+] Running 1/1
 ✔ Container collab-for-children-database-1  Started                                                                                                                                                                                     0.1s
[+] Building 0.0s (0/0)                                                                                                                                                                                                  docker:desktop-linux
Task started! View here:
https://console.aws.amazon.com/ecs/home?region=us-east-1#/clusters/ecsStagingCluster/tasks/6c871c1476a541f796e410ff453a3f36/details
arn:aws:ecs:us-east-1:ABCD1234:task/ecsStagingCluster/6c871c1476a541f796e410ff453a3f36

but that shouldn't affect any real world usage.

Testing Instructions

  • Check out https://github.com/azavea/collab-for-children/tree/release/test/375/import-larger-csvs
  • Build the container with the latest version of ecsmanage from this branch:
    $ docker compose build --no-cache
  • Add the following line locally:
    diff --git a/src/django/c4c/portal/views.py b/src/django/c4c/portal/views.py
    index 5d2c204..9b7e3d6 100644
    --- a/src/django/c4c/portal/views.py
    +++ b/src/django/c4c/portal/views.py
    @@ -861,6 +861,8 @@ def manage_showmigrations(request):
         # TODO Replace with CSV ingest command
         base_command = "showmigrations"
     
    +    settings.DJANGO_ENV = "staging"
    +
         if settings.DJANGO_ENV == "development":
             command = base_command
         elif settings.DJANGO_ENV == "staging":
  • Run ./scripts/update && ./scripts/server
  • Go to http://localhost:8080/manage/showmigrations/
    • Ensure you see the Task ARN and a link to the Task URL
    • Ensure you see the task run and complete at that URL

Copy link
Member

@aaronxsu aaronxsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Do we need to deploy it before it could be directly used?

@@ -207,4 +209,8 @@ def run_task(self, config, task_def_arn, security_group_id, subnet_id, cmd):
#
# See: https://docs.aws.amazon.com/AmazonECS/latest/userguide/ecs-account-settings.html#ecs-resource-ids # NOQA
task_id = task["taskArn"].split("/")[-1]
return task_id

return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a nitpick. It seems like task_id is read from task["taskArn"] anyway, so it might work if we just return task["taskArn"] and read the task_id in the calling spot of this run_task method. Functionality-wise, this makes no difference. So this change is optional and up to you!

Copy link
Collaborator Author

@rajadain rajadain Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great observation! Fixed in da01c53 aff275b.

@rajadain rajadain force-pushed the feature/return-task-arn-id branch 2 times, most recently from da01c53 to 201c7fd Compare January 5, 2024 22:26
When running ecsmanage in a programmatic setting, it is useful
to get the ARN of the task which it started, in addition to the
URL being printed to the console. This doesn't break existing
workflows, but adds the possibility of new ones.
@rajadain rajadain force-pushed the feature/return-task-arn-id branch from 201c7fd to aff275b Compare January 5, 2024 22:27
@rajadain
Copy link
Collaborator Author

rajadain commented Jan 5, 2024

Do we need to deploy it before it could be directly used?

Yes. I'll do a major version upgrade since this changes the API by returning a value where previously nothing was returned.

@rajadain rajadain merged commit f9ef03c into develop Jan 5, 2024
3 checks passed
@rajadain rajadain deleted the feature/return-task-arn-id branch January 5, 2024 22:35
@rajadain
Copy link
Collaborator Author

rajadain commented Jan 5, 2024

Thanks for reviewing and the great code suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants